Cast federation payload explicilty to an object #686
Conversation
|
Please add test then to avoid regressions in future |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
==========================================
- Coverage 90.74% 89.78% -0.97%
==========================================
Files 52 59 +7
Lines 1340 1449 +109
==========================================
+ Hits 1216 1301 +85
- Misses 124 148 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…hen you pass an empty MultiSearchFederation to the multiSearch function
…hen you pass an empty MultiSearchFederation to the multiSearch function
|
@thijskuilman not sure that we need a functional test here :) better would be to assert on that what was really passed to http client, because now a test without assertions doesn't prove anything |
|
IMHO we should a dev requirement for symfony/http-client, which allows easier testing than the guzzle. Then your test could look like this: code is just an idea and is not validated :) |
Yeah, I agree that it seems a bit weird that no assertions are done in the test. However, the current test does prove that no exceptions are raised during the function call: if you'd revert the object casting, then the test fails because the Beside that: I agree that testing the HTTP client instead might be nice (or a better solution) as well! :) |
|
My point was that unit test will be faster than the functional one :) |
| @@ -25,7 +25,7 @@ public function multiSearch(array $queries = [], ?MultiSearchFederation $federat | |||
|
|
|||
| $payload = ['queries' => $body]; | |||
| if (null !== $federation) { | |||
| $payload['federation'] = $federation->toArray(); | |||
| $payload['federation'] = (object) $federation->toArray(); | |||
There was a problem hiding this comment.
Why is typecasting necessary? You could use $payload['federation'] = $federation; and it would work just fine.
|
@curquiza Could you please review and process this PR or provide a solution? This is an important update. |
|
I've already reviewed and my point still stands - it'd enough for unit test (current test does not do any assertion) |
WalkthroughThe change updates how the 'federation' key is constructed in the payload for the Changes
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Endpoints/Delegates/HandlesMultiSearch.php(1 hunks)tests/Endpoints/HandlesMultiSearchTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Endpoints/HandlesMultiSearchTest.php (4)
src/Contracts/MultiSearchFederation.php (1)
MultiSearchFederation(7-94)src/Contracts/SearchQuery.php (2)
SearchQuery(7-495)setIndexUid(365-370)tests/TestCase.php (3)
TestCase(16-180)createEmptyIndex(139-145)safeIndexName(147-150)src/Endpoints/Delegates/HandlesMultiSearch.php (1)
multiSearch(18-32)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: integration-tests (PHP 8.4) (Kriswallsmith-Buzz)
- GitHub Check: integration-tests (PHP 8.4) (Guzzle-7-Adapter)
- GitHub Check: integration-tests (PHP 8.1) (PHP-HTTP-CurlClient)
- GitHub Check: integration-tests (PHP 8.0) (Kriswallsmith-Buzz)
🔇 Additional comments (1)
src/Endpoints/Delegates/HandlesMultiSearch.php (1)
28-28: Object casting is necessary for proper API compatibility.The explicit casting to
(object)is required because whenMultiSearchFederationhas no properties set,$federation->toArray()returns an empty array[]due to thearray_filter()call in thetoArray()method. However, the Meilisearch API expects an object structure{}rather than an array[]for the federation parameter.Without the cast, JSON encoding would produce:
{"queries": [...], "federation": []}With the cast, it correctly produces:
{"queries": [...], "federation": {}}This addresses the root cause mentioned in issue #685.
Responding to the past review comment: Direct assignment
$payload['federation'] = $federation;would not work because we need the array representation fromtoArray(), but cast as an object for proper JSON serialization.
| public function testPassingEmptyMultiSearchFederationDoesNotCauseException(): void | ||
| { | ||
| $this->expectNotToPerformAssertions(); | ||
|
|
||
| $this->client->multiSearch([ | ||
| (new SearchQuery())->setIndexUid($this->firstIndex->getUid()), | ||
| (new SearchQuery())->setIndexUid($this->secondIndex->getUid()), | ||
| ], | ||
| new MultiSearchFederation() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing unit tests with HTTP client assertions.
While this test ensures no exceptions are thrown, it lacks assertions on the actual behavior and uses a functional testing approach. As discussed in the PR comments, a more robust approach would be to unit test the HTTP client interactions directly.
Consider implementing the suggestion from the PR comments to mock the HTTP client and assert on the request payload:
public function testEmptyFederationPayloadIsCastToObject(): void
{
$httpClient = $this->createMock(Http::class);
$httpClient->expects($this->once())
->method('post')
->with(
'/multi-search',
$this->callback(function ($payload) {
// Verify federation is an object (stdClass) not an array
return isset($payload['federation'])
&& $payload['federation'] instanceof \stdClass;
})
);
$client = new Client('http://localhost:7700');
// Inject mocked HTTP client...
$client->multiSearch(
[(new SearchQuery())->setIndexUid('test')],
new MultiSearchFederation()
);
}This approach would:
- Verify the actual payload structure sent to the API
- Run faster as a unit test
- Provide more reliable regression protection
Would you like me to help implement a more comprehensive unit testing approach for this functionality?
🤖 Prompt for AI Agents
In tests/Endpoints/HandlesMultiSearchTest.php around lines 24 to 34, the current
test only checks that no exceptions occur but does not verify the actual HTTP
request payload. To improve, mock the HTTP client used by the multiSearch
method, set an expectation that the post method is called once with a payload
where the federation field is an object (stdClass), not an array. Then inject
this mocked client into the Client instance and call multiSearch with an empty
MultiSearchFederation. This will unit test the HTTP interaction and payload
structure more robustly.
|
|
||
| public function testPassingEmptyMultiSearchFederationDoesNotCauseException(): void | ||
| { | ||
| $this->expectNotToPerformAssertions(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove expectNotToPerformAssertions() and add meaningful assertions.
The expectNotToPerformAssertions() call indicates this test isn't actually verifying the fix works correctly - only that it doesn't throw exceptions. This makes the test less valuable for preventing regressions.
At minimum, consider adding an assertion that verifies the method completes successfully:
- $this->expectNotToPerformAssertions();
-
- $this->client->multiSearch([
+ $result = $this->client->multiSearch([
(new SearchQuery())->setIndexUid($this->firstIndex->getUid()),
(new SearchQuery())->setIndexUid($this->secondIndex->getUid()),
],
new MultiSearchFederation()
);
+
+ $this->assertIsArray($result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->expectNotToPerformAssertions(); | |
| $result = $this->client->multiSearch([ | |
| (new SearchQuery())->setIndexUid($this->firstIndex->getUid()), | |
| (new SearchQuery())->setIndexUid($this->secondIndex->getUid()), | |
| ], | |
| new MultiSearchFederation() | |
| ); | |
| $this->assertIsArray($result); |
🤖 Prompt for AI Agents
In tests/Endpoints/HandlesMultiSearchTest.php at line 26, remove the call to
expectNotToPerformAssertions() because it prevents the test from verifying
actual outcomes. Instead, add meaningful assertions that confirm the method
under test behaves as expected, such as checking the returned data structure,
status codes, or specific values to ensure the fix works correctly and prevent
regressions.
Strift
added
the
stale
Pull Request
Related issue
Fixes #685
What does this PR do?
federationpayload explicitly to an object. This solves the error that appears when noMultiSearchFederationproperties have been set.PR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
Bug Fixes
Tests